Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BnB: Add ability to specify integers #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

BnB: Add ability to specify integers #17

wants to merge 4 commits into from

Conversation

tuncbkose
Copy link

@tuncbkose tuncbkose commented Jun 20, 2024 •
edited
Loading

@henryrobbins Thanks for your response in #16. Here is what had put together, though I don't have a quick example to test (beyond the included tests).

I'm still not sure about what is happening in visualize.py, but I may take a look at it later. Meanwhile, I'd appreciate your comments on this.

One question about the slack variables: In the original code is it correct to do if np.sum(frac_comp) > 0: once slack variables are added? Could this not lead to a case where you are branching on a slack variable, which is not necessarily correct?

Todo:

  • Adjust LP to support specifying integrality
  • Change simplex and branch_and_bound to support integrality
  • Fix tests broken by the above
  • Refactor bnb_visual
  • Add more tests for MILPs

@tuncbkose tuncbkose marked this pull request as ready for review June 20, 2024 08:40
henryrobbins
henryrobbins reviewed Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@henryrobbins henryrobbins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the huge delay in reviewing this! Thanks for putting it together!

gilp/simplex.py Outdated Show resolved Hide resolved
gilp/simplex.py Outdated Show resolved Hide resolved
@henryrobbins
Copy link
Collaborator

I pulled your PR locally and tried it out on a 2D and 3D example. Seems to work!

Screenshot 2024-07-17 at 12 00 26 AM

@henryrobbins henryrobbins self-assigned this Jul 17, 2024
@henryrobbins henryrobbins added the enhancement New feature or request label Jul 17, 2024
@henryrobbins henryrobbins linked an issue Jul 17, 2024 that may be closed by this pull request
ENH: Generalizing the branch and bound implementation #16
Open
@tuncbkose
Copy link
Author

Thanks for the review! And no worries about the delay. I'll try to add more to this PR whenever I have the time.

@henryrobbins
Copy link
Collaborator

I still don't know if/what implications these may have for the visualization code because I couldn't get around to looking at it yet.

The branch_and_bound implementation and bnb_visual both call branch_and_bound_iteration. bnb_visual requires a lot of information from each iteration (current node, which constraints were added, remaining feasible regions, etc...). Because branch_and_bound only returned the final solution, it wasn't used in bnb_visual. That is resulting in a lot of duplicate code between branch_and_bound and bnb_visual. This should be refactored. One possibility is to include a "path" of the algorithm in the object returned by branch_and_bound that exposes all the necessary information for plotting (as is done with the simplex implementation. I'm open to other suggestions though.

@tuncbkose
Copy link
Author

I renamed int_mask to integrality. At the moment visualization is probably broken because it is calling simplex on the MILP rather than the relaxation. I'll leave fixing that to bnb_visual refactoring.

@tuncbkose
Copy link
Author

It's not a rewrite but I did add .get_relaxation() to all the simplex calls in bnb_visual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henryrobbins henryrobbins henryrobbins left review comments

Assignees

@henryrobbins henryrobbins

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ENH: Generalizing the branch and bound implementation
2 participants
@tuncbkose @henryrobbins